Make Modbus hub name mandatory by vzahradnik · Pull Request #37546 · home-assistant/core

vzahradnik · 2020-07-06T06:59:13Z

This pull request enforces hub name as a mandatory parameter. At the moment it's entirely optional, which introduces ambiguity.

Breaking change

Modbus hub name was optional. With this change, user has to specify a hub name
to avoid ambiguity in the configuration:

Set name parameter for each Modbus hub
Set hub parameter for each Modbus entity, telling which hub to use

Proposed change

Here's an example config:

modbus:
  - name: hub1
    type: tcp
    host: 127.0.0.1
    port: 5020
  - name: hub2
    type: tcp
    host: 127.0.0.1
    port: 5021

binary_sensor:
  - platform: modbus
    inputs:
      - name: Sensor1
        hub: hub1
        slave: 1
        address: 100

To better describe my point, let me show you the following several scenarios:

If we omit the hub parameter from the binary_sensor, this parameter will be set to default. Such a hub doesn't exist. Home Assistant throws an exception at some point while trying to access a default hub. We could add a code, which will set up an alias for the first hub in a list, but I think it's better to make the parameter mandatory instead
If we don't specify a name for hub1 and hub2, we end up with a non-working configuration. Both hubs are called default
By making the name mandatory, user has to name each hub and reference it in each Modbus entity. This approach should help avoiding configuration errors. Home Assistant config validator doesn't catch such errors - it would be only possible with cross-validation in place to validate configs between entities (i.e., if binary_sensor doesn't specify a hub, there should at least one Modbus hub with a default name. Otherwise, the config is invalid).

All examples in the documentation already specify a hub name. Users who copy and paste the sample documentation most likely use the name parameter already. However, if they're calling Modbus services write_coil and write_register from the automation, they will need to adapt their automation rules to also specify a hub.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Make Modbus hub name mandatory home-assistant.io#13920

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

Hub name in Modbus entities is optional. If not specified, hub is named 'default'. At the moment, users can omit hub name in entities like sensor, and if the hub has a non-default name like 'hub1', Home Assistant will throw an exception. It's not possible to do cross-validation. To avoid such errors and ambiguity, hub name will be mandatory.

stale · 2020-08-08T09:27:11Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

frenck · 2020-08-08T10:55:26Z

Not stale, awaits our review.

frenck · 2020-09-01T11:14:47Z

Been visiting this PR a couple of times now. I wonder why we should consider this. I get that it might have been a better idea to make the option mandatory from the get-go, but right now, it is optional and provides a default.

The only thing this PR will do, is causing a breaking change for existing users that currently DO rely on the default. So, why should we go and break this?

vzahradnik · 2020-09-01T12:35:15Z

There are three scenarios:

A user defines one Modbus hub, and he doesn't assign a name to it. Home Assistant will use the name "default"
A user defines one Modbus hub, and names it "hub1"; in the config section of sensors and switches he doesn't specify a hub name
A user defines two or more Modbus hubs
In the first case, everything will work. Added Modbus sensors, switches,... will use the hub with name "default".
In the second case, Home Assistant will access a non-existing hub with name "default" to poll for sensor and switch states. Config validation will pass (hub name is missing, but it's optional), and as soon as Home Assistant tries to poll for sensor data, it throws an error into a log. User needs to fix the config by specifying a proper hub name. However, for normal users, it might not be clear from reading all the exceptions
Third scenario throws the same errors as the second scenario.

The point of this PR is to enforce better config validation. Ideally, Home Assistant would support something like cross-validation where I could check in the sensors section the definitions of Modbus hubs, and notify users about misconfiguration. It's definitely better than throwing exceptions after validation has "passed".

Like you mentioned, this option should be probably mandatory from the start. While working on Modbus, I found several areas, where current configuration needs some improvement to avoid errors during runtime.

There are two options:

Introducing only backwards-compatible changes. As a result, the code will always be sub-optimal, because we cannot fix design decisions made years ago
Allowing to break backwards compatibility

While discussing in other PRs, I get your policy is to maintain backwards compatibility whenever possible. Anyway, I opened this PR to at least discuss the problem. If you think the current HA behavior is acceptable, you can close this PR.

Thanks!

frenck · 2020-09-01T15:15:45Z

The point of this PR is to enforce better config validation.

I'm not against that, however, breaking perfectly working existing configuration without a darn good reason to do so, is not.

I think this change would make sense if it was converted from separate platform components into an actual integration (as that would combine the breaking changes).

vzahradnik · 2020-09-01T15:51:33Z

Ok, in that case I suggest to close this PR. I will also try to get my other PRs merged (as time permits) in a backwards-compatible way. Then, we can start a discussion, how to start a transition into a proper integration.

home-assistant#37546) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: smarthome-10 <monkey10github@protonmail.com> Co-authored-by: Darren Griffin <darren.griffin@live.co.uk> Co-authored-by: jey burrows <jrb@jeymail.net> Co-authored-by: Manu <4445816+tr4nt0r@users.noreply.github.com> Co-authored-by: Nic Eggert <nic@eggert.io> Co-authored-by: GilDev <gildev@gmail.com> Co-authored-by: c0ffeeca7 <38767475+c0ffeeca7@users.noreply.github.com> Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com> Co-authored-by: cdnninja <jaydenaphillips@gmail.com> Co-authored-by: Klaas Schoute <klaas_schoute@hotmail.com> Co-authored-by: Franck Nijhof <git@frenck.dev> Co-authored-by: Michael <35783820+mib1185@users.noreply.github.com> Co-authored-by: Danny Tsang <567982+dannytsang@users.noreply.github.com> Co-authored-by: Pete Sage <76050312+PeteRager@users.noreply.github.com> Co-authored-by: Jean-Marc Collin <jm.collin.78@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: hanwg <han.wuguang@gmail.com> Co-authored-by: Brett Adams <Bre77@users.noreply.github.com> Co-authored-by: Dan Long <dannylong1988@gmail.com> Co-authored-by: Mitchell van Manen <ma.van.manen@live.nl> Co-authored-by: karwosts <32912880+karwosts@users.noreply.github.com> Co-authored-by: Thomas D <11554546+thomasddn@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Maikel Punie <maikel.punie@gmail.com> Co-authored-by: Petro31 <35082313+Petro31@users.noreply.github.com> Co-authored-by: Michel van de Wetering <michel.van.de.wetering@gmail.com> Co-authored-by: Maciej Bieniek <bieniu@users.noreply.github.com> Co-authored-by: Shay Levy <levyshay1@gmail.com> Co-authored-by: Jérôme OUDOUL <jerome@oudoul.com> Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com> Co-authored-by: jkb543 <245537795+jkb543@users.noreply.github.com> Co-authored-by: Josef Zweck <josef@zweck.dev> Co-authored-by: ildar170975 <71872483+ildar170975@users.noreply.github.com> Co-authored-by: Marland Sitt <marland.sitt@gmail.com> Co-authored-by: Nc Hodges <86037210+Hodnc@users.noreply.github.com> Co-authored-by: Steve Easley <steve.easley@gmail.com> Co-authored-by: Ludovic BOUÉ <lboue@users.noreply.github.com> Co-authored-by: MinchinWeb <w_minchin@hotmail.com> Co-authored-by: Anton Dalgren <antondalgren@users.noreply.github.com> Co-authored-by: Duco Sebel <74970928+DCSBL@users.noreply.github.com> Co-authored-by: Andrew Jackson <andrew@codechimp.org> Co-authored-by: Frederic Mariën <FredericMa@users.noreply.github.com> Co-authored-by: Allen Porter <allen.porter@gmail.com> Co-authored-by: Retha Runolfsson <137745329+zerzhang@users.noreply.github.com> Co-authored-by: johanzander <johanzander@gmail.com> Co-authored-by: Paul Tarjan <github@paulisageek.com> Co-authored-by: Jan Bouwhuis <jbouwh@users.noreply.github.com> Co-authored-by: Jordan Harvey <jordan@hrvy.uk> Co-authored-by: TheJulianJES <TheJulianJES@users.noreply.github.com> Co-authored-by: Abílio Costa <abmantis@users.noreply.github.com> Co-authored-by: Angel <mail2angel17@gmail.com> Co-authored-by: Anthony Garera <gareraanthony@gmail.com> Co-authored-by: starkillerOG <starkiller.og@gmail.com> Co-authored-by: Hedda <rockerc.harley@gmail.com> Co-authored-by: Robert Resch <robert@resch.dev> Co-authored-by: Kurt Chrisford <92524101+kclif9@users.noreply.github.com> Co-authored-by: theobld-ww <60600399+theobld-ww@users.noreply.github.com> Co-authored-by: Hans Fehrmann <hans.jfehrmann@gmail.com> Co-authored-by: Matthias Alphart <farmio@alphart.net> Co-authored-by: Niracler <i@niracler.com> Co-authored-by: Siemon Geeroms <siemongeeroms@gmail.com> Co-authored-by: rlippmann <70883373+rlippmann@users.noreply.github.com> Co-authored-by: Lukas <12813107+lmaertin@users.noreply.github.com> Co-authored-by: Ravaka Razafimanantsoa <3774520+SeraphicRav@users.noreply.github.com> Co-authored-by: Guido Schmitz <Shutgun@users.noreply.github.com> Co-authored-by: Bram Kragten <mail@bramkragten.nl> Co-authored-by: Markus Jacobsen <markusjacobsen@hotmail.com> Co-authored-by: wollew <wollew@users.noreply.github.com> Co-authored-by: Marc Hörsken <mback2k@users.noreply.github.com> Co-authored-by: Chris Boyle <chris@boyle.name> Co-authored-by: Marc-Philip <marc-philip.werner@sap.com> Co-authored-by: dvfeinblum <dvfeinblum@gmail.com> Co-authored-by: Aviad Levy <aviadlevy1@gmail.com> Co-authored-by: Martin Hjelmare <marhje52@gmail.com> Co-authored-by: mettolen <1007649+mettolen@users.noreply.github.com> Co-authored-by: W7RZL <kevmacmills@gmail.com> Co-authored-by: Lukas Gill <lukas.gill@me.com> Co-authored-by: Lukas Gill <gitlab@lukasgill.xyz> Co-authored-by: Galorhallen <12990764+Galorhallen@users.noreply.github.com> Co-authored-by: Ted Pennings <310323+tedpennings@users.noreply.github.com> Co-authored-by: Louis Christ <LouisChrist@users.noreply.github.com> Co-authored-by: Ville Skyttä <ville.skytta@iki.fi> Co-authored-by: Simon Lamon <32477463+silamon@users.noreply.github.com> Co-authored-by: Jason Stirnaman <stirnamanj@gmail.com> Co-authored-by: Didgeridrew <19187320+Didgeridrew@users.noreply.github.com> Co-authored-by: doomsniper09 <79980810+doomsniper09@users.noreply.github.com> Co-authored-by: jameson_uk <1040621+jamesonuk@users.noreply.github.com> Co-authored-by: Raphael Hehl <7577984+RaHehl@users.noreply.github.com> Co-authored-by: Trainmaster2 <dphammock@gmail.com> Co-authored-by: Andrzej Dopierała <theundefined@users.noreply.github.com> Co-authored-by: Jan Klausa <jan@klausa.pl> Co-authored-by: MarkGodwin <10632972+MarkGodwin@users.noreply.github.com> Co-authored-by: surfingbytes <94438335+surfingbytes@users.noreply.github.com> Co-authored-by: Matrix <justin@yosmart.com> Co-authored-by: J. Diego Rodríguez Royo <jdrr1998@hotmail.com> Co-authored-by: Glenn de Haan <glenn@dehaan.cloud> Co-authored-by: Willem-Jan van Rootselaar <liudgervr@gmail.com> Co-authored-by: Felipe Santos <felipecassiors@gmail.com> Co-authored-by: Åke Strandberg <ake@strandberg.eu>

homeassistant added the cla-signed label Jul 6, 2020

probot-home-assistant Bot added breaking-change integration: modbus small-pr PRs with less than 30 lines. labels Jul 6, 2020

vzahradnik mentioned this pull request Jul 6, 2020

Make Modbus hub name mandatory home-assistant/home-assistant.io#13920

Closed

8 tasks

vzahradnik mentioned this pull request Jul 6, 2020

Add Modbus cover #33642

Merged

20 tasks

stale Bot added the stale label Aug 8, 2020

stale Bot removed the stale label Aug 8, 2020

frenck closed this Sep 3, 2020

vzahradnik deleted the bugfix/remove-default-hub branch September 3, 2020 18:19

vzahradnik mentioned this pull request Sep 28, 2020

Add test coverage for modbus switch (coil part) #40696

Merged

14 tasks

janiversen mentioned this pull request Sep 28, 2020

Update modbus config to a central structure and update to get silver quality. #40705

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make Modbus hub name mandatory#37546

Make Modbus hub name mandatory#37546
vzahradnik wants to merge 1 commit intohome-assistant:devfrom
Lutemi:bugfix/remove-default-hub

vzahradnik commented Jul 6, 2020 •

edited

Loading

Uh oh!

stale Bot commented Aug 8, 2020

Uh oh!

frenck commented Aug 8, 2020

Uh oh!

frenck commented Sep 1, 2020

Uh oh!

vzahradnik commented Sep 1, 2020

Uh oh!

frenck commented Sep 1, 2020

Uh oh!

vzahradnik commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

vzahradnik commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Additional information

Checklist

Uh oh!

stale Bot commented Aug 8, 2020

Uh oh!

frenck commented Aug 8, 2020

Uh oh!

frenck commented Sep 1, 2020

Uh oh!

vzahradnik commented Sep 1, 2020

Uh oh!

frenck commented Sep 1, 2020

Uh oh!

vzahradnik commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vzahradnik commented Jul 6, 2020 •

edited

Loading